-
Notifications
You must be signed in to change notification settings - Fork 273
Use help_formatter to consistently format help output #6931
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
d88808b
to
7df479e
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #6931 +/- ##
===========================================
+ Coverage 78.80% 78.82% +0.02%
===========================================
Files 1698 1699 +1
Lines 195118 195102 -16
===========================================
+ Hits 153757 153798 +41
+ Misses 41361 41304 -57
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff but be aware of the potential revert of #6932
"(vsd-liveness)" \ | ||
\ | ||
// clang-format off | ||
"(vsd-liveness)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we still doing the "put a \ at the end of even the last line so that new additions are just 1-line diffs" thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be useful, but clang-format takes them away. So I'd have to (re-)add clang-format off/on
, which makes formatting more cumbersome. I'm not sure which is best, happy to take guidance!
" --vsd-liveness track variable liveness\n" \ | ||
|
||
// cland-format on | ||
help_entry( \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel a bit weird about having the <<
operators for all but the first. I guess we have to out << HELP_VSD
which is grammatical but ... it feels a bit weird.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to move the <<
over into the macros, but I don't know whether it actually makes for less weirdness. I tripped over this myself when coming up with this patch...
7df479e
to
e3a0992
Compare
e3a0992
to
a2c2626
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Michael.
This looks tedious, but it's a very important step to establish some uniformity and a standard around the help options.
Thanks for taking the time to sort this out.
Pinging @chris-ryder as I believe I'll strictly need your approval here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes in contracts
LGTM.
"(object-bits):" \ | ||
"(gcc)" | ||
|
||
#define HELP_CONFIG_PLATFORM \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since help_entry is a function could we instead write these as c++ functions returning std::string
and avoid the multi-line macros ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the multi-line macros that just expand chais of calls to help_entry
and concatenation using <<
(see #define HELP_CONFIG_LIBRARY
for instance) really needed or could we just use C++ functions returning std::string
everywhere ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the multi-line macros that just expand chais of calls to help_entry
and concatenation using <<
(see #define HELP_CONFIG_LIBRARY
for instance) really needed or could we just use C++ functions returning std::string
everywhere ?
I'll work on this. |
I should likely rebase/rework this to use #7141. |
a2c2626
to
54e96da
Compare
54e96da
to
11c0865
Compare
11c0865
to
376d852
Compare
To anyone reviewing this: on this occasion, reviewing the overall diff will be more effective than doing individual commits (I'm mainly keeping the "Use help_entry" commit for sentimental reasons). |
Modelled after help_entry's unit test.
We previously did manual line breaks that either suited the source code line length or approximated the output line width. help_entry, however, was specifically built to take care of this (and just wasn't widely used). We can then also happily have clang-format take care of source code formatting without impacting the resulting display.
First round of changes was done automatically: ``` sed -i 's/help_entry("\(--[^ ]*\)", "\(.*\)")/" {y\1} \\t \2\\n"/' \ src/*/*.h src/*/*.cpp src/analyses/variable-sensitivity/*.h \ jbmc/src/*/*.h jbmc/src/*/*.cpp ``` All further replacements were done manually.
All uses of `help_entry` have been converted to uses of `help_formatter`.
376d852
to
722918f
Compare
We previously did manual line breaks that either suited the source code
line length or approximated the output line width. help_formatter, however,
was specifically built to take care of this (and just wasn't widely
used). We can then also happily have clang-format take care of source
code formatting without impacting the resulting display.